Skip to content

fix(cli): stop the helper commands destroying files they do not own - #145

Merged
LeadcodeDev merged 1 commit into
chantier/audit-remediationfrom
fix/audit-lot3a-destructive-commands
Aug 8, 2026
Merged

fix(cli): stop the helper commands destroying files they do not own#145
LeadcodeDev merged 1 commit into
chantier/audit-remediationfrom
fix/audit-lot3a-destructive-commands

Conversation

@LeadcodeDev

Copy link
Copy Markdown
Owner

Refs #142 — CLI workstream, first of three. Four confirmed High findings, all of them cases where a helper command destroys user-authored content.

skills install / uninstall vs. the project's CLAUDE.md

install wrote CLAUDE.md wholesale; uninstall called remove_file on it unconditionally. Both treat a file the user authors as rustmotion's property.

Reproduced in a clean directory holding # My Project / Build with make. Never delete this.:

  • before — skills install replaced it outright, skills uninstall deleted it;
  • after — install merges into a <!-- rustmotion:start -->…<!-- rustmotion:end --> block leaving the surrounding content intact, and uninstall removes only that block, keeping the file. The file is deleted only when our block was all it ever held.

Re-installing replaces the block in place rather than stacking copies, which the previous whole-file write got right by accident and a naive append would have got wrong.

--fix, three defects and one cause

--fix serialises LoadedScenario::raw — documented in validation.rs:36 as the scenario after parsing, variable resolution and include resolution. That is not the document on disk. For a plain JSON scenario the two coincide and writing back is faithful; for anything templated it is not, and the write replaces the source with its own expansion.

The audit filed these as three findings; they are one bug seen from three angles:

Input What --fix wrote back
.html the transpiled JSON, replacing the author's markup
scenario with config / $var the substituted scenario — template gone, --var becomes a silent no-op
scenario with include the resolved tree, inlining the included files and patching by a path the source no longer contains

So the fix is one rule, not three patches: only write back a source --fix can reproduce. Anything else is refused, with a message naming the file and saying what to do instead.

The guard reads the bytes on disk rather than the loaded tree — by the time the tree exists, substitution has already erased the very markers that make the write unfaithful.

Verified end to end: validate -f f.html --fix now refuses and leaves the HTML byte-identical, where it previously overwrote it with JSON.

Design note

refuse_fix returns a typed FixRefusal rather than a bool so each case carries its own explanation; explain() is tested to always name the file and offer a next step, since a refusal with no way forward is just a different kind of dead end.

Out of scope

batch (exit code, geometry bypass, path traversal) and the HTML dialect findings (<style> painted into the video, nested <scene> dropped, unknown tags degraded to div, boolean coercion) are the two remaining CLI PRs in this chantier. Splitting them keeps each reviewable in one sitting.

Verification

cargo fmt --all --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test --workspace (702 passed, 0 failed). 10 new tests: 5 on the CLAUDE.md merge, 5 on the fix guard.

`skills install` wrote CLAUDE.md wholesale and `skills uninstall` deleted
it, treating a file the user authors as rustmotion's property: a project
with its own build notes lost them on install and lost the file on
uninstall. rustmotion now claims a delimited block and never touches
anything outside it — install merges the block in place, uninstall removes
only the block and deletes the file solely when that block was all it held.

`--fix` serialises `LoadedScenario::raw`, which is the document *after*
variable substitution and include resolution — not the document on disk.
For a plain JSON scenario the two coincide; for anything templated the
write silently replaces the source with its own expansion. That single
cause produced three separate defects: an HTML input came back as JSON, a
`config` block and every `$var` disappeared (making `--var` a no-op on the
rewritten file), and `include` got inlined so path-based patches landed on
nodes the source never contained.

One rule closes all three: only write back a source `--fix` can reproduce.
Anything else is refused with a message naming the file and what to do
instead. The check reads the bytes on disk rather than the loaded tree,
because by then substitution has already erased the markers that make the
write unfaithful.
@LeadcodeDev LeadcodeDev added the bug Something isn't working label Aug 8, 2026
@LeadcodeDev LeadcodeDev self-assigned this Aug 8, 2026
@LeadcodeDev
LeadcodeDev merged commit 69ba2ab into chantier/audit-remediation Aug 8, 2026
3 checks passed
@LeadcodeDev
LeadcodeDev deleted the fix/audit-lot3a-destructive-commands branch August 8, 2026 12:15
LeadcodeDev added a commit that referenced this pull request Aug 9, 2026
…plementing it (#160)

Nine confirmed audit findings on the geometry pass. Almost all of them are
the same structural mistake: the validator forked a piece of the engine — a
padding constant, a root style, a transform pivot — instead of calling the
function the engine uses. Then the two drifted, and the validator started
reasoning about a geometry that is not the one being rendered. The fix is
mostly deletion of the duplicate, not correction of its value.

False negatives — a broken video shipped with no warning:

- `timeline` style states and audio-reactive transforms were invisible even
  under `--strict-anim`: the box tree was built once with `anim: None`, and
  both features only apply when a real animation context is present. It is
  now rebuilt per sample through `build_scene_from_refs` with a real
  `BuildAnimationCtx` — the same call `render_with_new_pipeline_iter` makes
  per rendered frame.
- Animated rotation was not modelled at all: `transform_bbox` read only
  translate and scale, so a `spin` left the frame undetected at every
  sample. It now folds through `apply_static_node_transform`, the same
  4-corner AABB the static path already used.
- The 40-sample cap dropped `--strict-anim` to one sample every 1.5s on a
  60s scene, so any excursion shorter than the step slipped through. Raised
  to 480, which holds the promised 8/s up to 60s. Measured cost on a 60s
  scene with 15 animated components: 309ms release, 543ms debug.

False positives — a correct scenario blocked, and `--fix` then damaging it:

- `unwrappable_text_overflow` was the only content-vs-own-box check outside
  the clipping-ancestor guard, so text legitimately clipped by an
  `overflow: hidden` ancestor was rejected and `--fix` stripped a correct
  `white-space`.
- `transform-origin` was ignored by the static transform fold, which always
  pivoted at the box centre.
- `content_overflows_card` is retired rather than patched. Its guard made it
  reachable only when neither the card nor anything between it and the node
  clips — which is exactly the `overflow: visible` case both CLAUDE.md and
  geometry-safety.md document as legal ("a badge sticking out of a card is
  legal"). When the card does clip, the guard already suppresses the whole
  block. There is no configuration where firing is both reachable and
  consistent with the documented contract. Content leaving the *device* is
  still caught — that is `check_viewport`'s job, and a test now pins that
  guarantee so the removal cannot silently become a blind spot.

Wrong geometry reported:

- `world` views were validated against the slide root layout. The centred
  default the world renderer synthesises is now a single shared
  `world_default_scene_layout`, used by both.
- `check_auto_scroll` assumed 16+16px of padding and, for terminal, the
  wrong default font size and a CSS line-height the painter ignores. It now
  calls `CodeblockIntrinsic` and `TerminalIntrinsic`, the measurers layout
  itself uses.

The `--fix` refusals from PR #145 were verified rather than rewritten: two
end-to-end tests now drive `cmd_validate` over a templated scenario and a
two-file `include` scenario, both with a real violation, and assert the
source files come back byte-identical. Doing better than refusing is not
possible for `include`, and not safe for templates: variable substitution
can replace a scalar with an object, so a path computed on the resolved tree
need not exist in the source.

Tests: full workspace green on this branch alone. 7/8 examples validate; the
eighth is issue #157, pre-existing on main.
LeadcodeDev added a commit that referenced this pull request Aug 10, 2026
…145)

`skills install` wrote CLAUDE.md wholesale and `skills uninstall` deleted
it, treating a file the user authors as rustmotion's property: a project
with its own build notes lost them on install and lost the file on
uninstall. rustmotion now claims a delimited block and never touches
anything outside it — install merges the block in place, uninstall removes
only the block and deletes the file solely when that block was all it held.

`--fix` serialises `LoadedScenario::raw`, which is the document *after*
variable substitution and include resolution — not the document on disk.
For a plain JSON scenario the two coincide; for anything templated the
write silently replaces the source with its own expansion. That single
cause produced three separate defects: an HTML input came back as JSON, a
`config` block and every `$var` disappeared (making `--var` a no-op on the
rewritten file), and `include` got inlined so path-based patches landed on
nodes the source never contained.

One rule closes all three: only write back a source `--fix` can reproduce.
Anything else is refused with a message naming the file and what to do
instead. The check reads the bytes on disk rather than the loaded tree,
because by then substitution has already erased the markers that make the
write unfaithful.
LeadcodeDev added a commit that referenced this pull request Aug 10, 2026
…plementing it (#160)

Nine confirmed audit findings on the geometry pass. Almost all of them are
the same structural mistake: the validator forked a piece of the engine — a
padding constant, a root style, a transform pivot — instead of calling the
function the engine uses. Then the two drifted, and the validator started
reasoning about a geometry that is not the one being rendered. The fix is
mostly deletion of the duplicate, not correction of its value.

False negatives — a broken video shipped with no warning:

- `timeline` style states and audio-reactive transforms were invisible even
  under `--strict-anim`: the box tree was built once with `anim: None`, and
  both features only apply when a real animation context is present. It is
  now rebuilt per sample through `build_scene_from_refs` with a real
  `BuildAnimationCtx` — the same call `render_with_new_pipeline_iter` makes
  per rendered frame.
- Animated rotation was not modelled at all: `transform_bbox` read only
  translate and scale, so a `spin` left the frame undetected at every
  sample. It now folds through `apply_static_node_transform`, the same
  4-corner AABB the static path already used.
- The 40-sample cap dropped `--strict-anim` to one sample every 1.5s on a
  60s scene, so any excursion shorter than the step slipped through. Raised
  to 480, which holds the promised 8/s up to 60s. Measured cost on a 60s
  scene with 15 animated components: 309ms release, 543ms debug.

False positives — a correct scenario blocked, and `--fix` then damaging it:

- `unwrappable_text_overflow` was the only content-vs-own-box check outside
  the clipping-ancestor guard, so text legitimately clipped by an
  `overflow: hidden` ancestor was rejected and `--fix` stripped a correct
  `white-space`.
- `transform-origin` was ignored by the static transform fold, which always
  pivoted at the box centre.
- `content_overflows_card` is retired rather than patched. Its guard made it
  reachable only when neither the card nor anything between it and the node
  clips — which is exactly the `overflow: visible` case both CLAUDE.md and
  geometry-safety.md document as legal ("a badge sticking out of a card is
  legal"). When the card does clip, the guard already suppresses the whole
  block. There is no configuration where firing is both reachable and
  consistent with the documented contract. Content leaving the *device* is
  still caught — that is `check_viewport`'s job, and a test now pins that
  guarantee so the removal cannot silently become a blind spot.

Wrong geometry reported:

- `world` views were validated against the slide root layout. The centred
  default the world renderer synthesises is now a single shared
  `world_default_scene_layout`, used by both.
- `check_auto_scroll` assumed 16+16px of padding and, for terminal, the
  wrong default font size and a CSS line-height the painter ignores. It now
  calls `CodeblockIntrinsic` and `TerminalIntrinsic`, the measurers layout
  itself uses.

The `--fix` refusals from PR #145 were verified rather than rewritten: two
end-to-end tests now drive `cmd_validate` over a templated scenario and a
two-file `include` scenario, both with a real violation, and assert the
source files come back byte-identical. Doing better than refusing is not
possible for `include`, and not safe for templates: variable substitution
can replace a scalar with an object, so a path computed on the resolved tree
need not exist in the source.

Tests: full workspace green on this branch alone. 7/8 examples validate; the
eighth is issue #157, pre-existing on main.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant